@geoand can you fix the conflict?

@stuartwdouglas will you have time to review or should we find another reviewer?

@FroMage done :)

Thanks

On the face of it I think it looks ok. Do we have any benchmarks on how much it improves performance (both runtime perf an memory usage when you have a lot of return types)?

@stuartwdouglas I had done some simple JMH benchmarking with some very simple and very complex types. For the simple ones the performance improvement was very small, for the very complex ones the improvements could go up to 80%.
As for memory usage, unfortunately I haven't tested it. I'll try and set up some scenarios next week.

I haven't yet been able to run the JMH and memory tests, hopefully today or tomorrow

This needs to be reworked now that we have JSON-B CDI support in master (as a result of #4212)

Any update on this one?

This has been put on hold in order to do more important things

@geoand I have a somewhat naive question on this one: shouldn't JSON-B cache the serializers somehow? I'm wondering if we use it correctly.

/cc @aguibert

@gsmet as part of this PR I had found a way to get really deep integration of Serializers into JSON-B.
The code was fairly involved and hasn't been moved into our regular serializer handling

Yeah sure. But what I'm wondering is: should the JSON-B performances be that bad? If JSON-B is caching the serializers it generates, I find the 80% improvement to be suspicious (not that I doubt your assertions, I'm rather wondering if either JSON-B or we are doing something wrong).

Might be a good idea to have @aguibert 's opinion on this.

thanks for looping me in @gsmet. I've had a look at this PR and spoke with Georgios. To summarize:

1. My main concern with this PR is that it's essentially re-implementing all of the built-in primitive serializers, which is going to be a lot of code to maintain and may cause Quarkus w/ Yasson behavior to diverge from pure Yasson behavior. Also, we would need to start running the JSON-B TCK w/ Quarkus since we'd be adding a significant amount of code that had the ability to impact behavior.
2. Overriding this much of Yasson behavior would exclude us from natural performance improvements that will happen in Yasson over time. For example (although not directly applicable here) one of my Yasson colleagues is currently working on a new deserialization algorithm that will improve deser perf ~80%.

Currently the most expensive operation Yasson does is parsing class models using reflection. This is only done once when Yasson first touches a given Java model class. However, this class parsing is done lazily so it may impact first-request time.

I think we should start by adding an SPI to Yasson where we can pass in a list of model classes to be eagerly initialized. This way the expensive class parsing code runs at build-time instead of on first-request. I've raised the following issue to track this enhancement in Yasson: eclipse-ee4j/yasson#366

That was exactly my line of thought.

@geoand when you did your testing, it was after warmup or did you always regenerate the Yasson metadata?

IIRC it was after warmup.

@aguibert could you check that we are doing things correctly in Quarkus/RESTEasy and that we correctly cache the JSON-B instance?

yea I'll take a look at what we are currently doing. @geoand I assume you were doing a JAX-RS w/ JSONB benchmark scenario?

@aguibert I did both a JAX-RS w JSON-B load test and microbenchmarks with just JSON-B

I'm going to close this one for now. I would prefer @aguibert to lead the effort of checking if we are doing things correctly regarding JSON-B and also optimize things in Yasson itself.

We can reopen if we think it's worth it at some point.